-
Notifications
You must be signed in to change notification settings - Fork 189
[rocBLAS] Refactor memory management in host_alloc.cpp #3672
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Updated the memory deallocation logic in free_ptr_use to use an iterator for accessing allocated memory. This change improves code clarity and efficiency by directly checking the existence of the pointer in the mem_allocated map and adjusting the memory usage accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the memory deallocation logic in free_ptr_use() to use iterator-based map access, improving both efficiency and correctness. The change eliminates redundant map lookups and prevents unintended insertions that could occur with operator[].
Key Changes:
- Replaced three sequential map lookups with a single
find()call using an iterator - Used the cached iterator for subsequent operations (accessing size and erasing entry)
- Eliminated risk of unintended map insertions when checking for pointer existence
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## develop #3672 +/- ##
============================================
- Coverage 80.14% 46.04% -34.10%
============================================
Files 76 365 +289
Lines 3323 51594 +48271
Branches 0 5964 +5964
============================================
+ Hits 2663 23752 +21089
- Misses 660 24359 +23699
- Partials 0 3483 +3483
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
TorreZuk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve but my suggestions I usually want considered to be made before merge :-)
My style is I only don't approve if I think the merge would cause trouble as is.
… users/todavis/host_alloc_fix
…reeing untracked pointers.
… users/todavis/host_alloc_fix
|
Looks like you have to run clang format again, otherwise only baseline fails. |
[rocBLAS] Refactor memory management in host_alloc.cpp
(#3672)
## Summary
Improved memory management in `host_alloc.cpp` by refactoring the
deallocation logic for better efficiency and adding diagnostic warnings
for untracked pointer frees.
## Changes
1. **Refactored `free_ptr_use()` to use iterator-based access** (reduces
redundant map lookups)
2. **Added warning for freeing untracked pointers** (helps detect
potential memory corruption issues)
## Motivation
The current implementation of `free_ptr_use()` in `host_alloc.cpp` had
two issues:
1. **Performance:** Redundant map lookups when deallocating tracked
memory
2. **Correctness:** Using `operator[]` on a map can unintentionally
insert entries for non-existent keys
3. **Diagnostics:** Silent failures when attempting to free untracked
pointers made debugging difficult
This PR addresses all three issues by refactoring the lookup logic and
adding diagnostic warnings.
## Technical Details
**File:** `projects/rocblas/clients/common/host_alloc.cpp`
### Change 1: Iterator-based memory deallocation
**Before:**
```cpp
if(ptr && mem_allocated[ptr]) { // Lookup #1 (may insert!)
mem_used -= mem_allocated[ptr]; // Lookup #2
mem_allocated.erase(ptr); // Lookup #3
}
```
**After:**
```cpp
auto it = mem_allocated.find(ptr); // Single lookup
if(ptr && it != mem_allocated.end()) {
mem_used -= it->second; // Use cached iterator
mem_allocated.erase(it); // Use cached iterator
}
```
**Benefits:**
- Reduces map lookups from 3 to 1
- Prevents unintended map insertions via `operator[]`
- More efficient memory tracking
### Change 2: Diagnostic warning for untracked pointers
**Added:**
```cpp
else if(ptr && call_free)
{
rocblas_cerr << "Warning: Freeing untracked pointer " << ptr
<< " - untracked memory released (potential double-free or memory corruption)"
<< std::endl;
}
```
**Benefits:**
- Helps detect double-free bugs
- Identifies potential memory corruption issues
- Provides actionable diagnostic information during testing
## Test Plan
- Built rocBLAS with no compilation errors
- Existing client test suite exercises memory allocation/deallocation
paths
- No functional behavior changes to normal operation
- Warning messages help identify issues during debugging
## Test Result
- All existing tests continue to pass
- Memory tracking functionality unchanged
- Warning properly triggers for untracked pointer frees
- No behavioral differences for correctly tracked memory
## Submission Checklist
- [x] Look over the contributing guidelines at
https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
…hanges CMakeLists.txt changes: - Inline BLAS detection functions (removes 185 lines of helper functions) - Keep find_libblis() as function (called twice) - Remove AOCL_ROOT and HOME/aocl support (scope creep) - Remove fancy formatting (checkmarks, separators, summary box) - Net: +59 lines vs develop (only 11 more than minimal approach) Also brings in develop fixes: - host_alloc.cpp: Better memory tracking and double-free detection (#3672) - test files: Minor test updates from develop - Linux_Install_Guide.rst: Documentation sync
Summary
Improved memory management in
host_alloc.cppby refactoring the deallocation logic for better efficiency and adding diagnostic warnings for untracked pointer frees.Changes
free_ptr_use()to use iterator-based access (reduces redundant map lookups)Motivation
The current implementation of
free_ptr_use()inhost_alloc.cpphad two issues:operator[]on a map can unintentionally insert entries for non-existent keysThis PR addresses all three issues by refactoring the lookup logic and adding diagnostic warnings.
Technical Details
File:
projects/rocblas/clients/common/host_alloc.cppChange 1: Iterator-based memory deallocation
Before:
After:
Benefits:
operator[]Change 2: Diagnostic warning for untracked pointers
Added:
Benefits:
Test Plan
Test Result
Submission Checklist